Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

modules: runtime deprecate subpath folder mappings #35747

Closed
wants to merge 8 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Oct 22, 2020

This creates an explicit runtime deprecation warning when using the folder mappings in "exports" or "imports".

The implicit documentation deprecation was already made when the exports patterns PR was landed in #34718 and fully replaced all documentation references to this.

The warning is carefully scoped to not apply to external packages in node_modules, unless explicitly using the --pending-deprecation flag.

Tests are included for local and external, require and import, imports and exports and self-resolution cases.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
  • documentation is changed or added

@nodejs-github-bot nodejs-github-bot added the esm Issues and PRs related to the ECMAScript Modules implementation. label Oct 22, 2020
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@guybedford guybedford force-pushed the deprecate-export-trailer branch 2 times, most recently from 2d4f98e to 6e21041 Compare October 22, 2020 00:47
@codecov-io
Copy link

codecov-io commented Oct 22, 2020

Codecov Report

Merging #35747 into master will decrease coverage by 8.49%.
The diff coverage is 98.03%.

@@            Coverage Diff             @@
##           master   #35747      +/-   ##
==========================================
- Coverage   96.41%   87.91%   -8.50%     
==========================================
  Files         223      477     +254     
  Lines       73685   113133   +39448     
  Branches        0    24642   +24642     
==========================================
+ Hits        71043    99462   +28419     
- Misses       2642     7963    +5321     
- Partials        0     5708    +5708     
Impacted Files Coverage Δ
lib/internal/modules/esm/resolve.js 93.65% <98.03%> (-1.09%) ⬇️
lib/internal/idna.js 55.55% <0.00%> (-11.12%) ⬇️
lib/internal/blocklist.js 88.70% <0.00%> (-10.49%) ⬇️
lib/internal/crypto/mac.js 73.45% <0.00%> (-9.48%) ⬇️
lib/internal/modules/esm/get_format.js 84.72% <0.00%> (-8.34%) ⬇️
lib/internal/crypto/aes.js 84.21% <0.00%> (-6.44%) ⬇️
lib/internal/crypto/dsa.js 85.28% <0.00%> (-6.04%) ⬇️
lib/internal/crypto/webcrypto.js 84.60% <0.00%> (-5.40%) ⬇️
lib/internal/histogram.js 79.78% <0.00%> (-5.32%) ⬇️
lib/internal/dtrace.js 95.23% <0.00%> (-4.77%) ⬇️
... and 390 more

lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@addaleax addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 22, 2020
doc/api/deprecations.md Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Oct 22, 2020

@addaleax This is a runtime deprecation on two experimental features (subpath exports and subpath imports), so maybe it's not semver-major?

@addaleax
Copy link
Member

@Trott Yeah, no strong opinion, it felt like it’s better to err on the safe side here to me, that’s all :)

@guybedford
Copy link
Contributor Author

Thinking some more about the backporting I wonder if it might actually be useful to backport this to 14.

The warnings will only show for local package usage since we have good filtering to not show for third-party packages without -pending-deprecation.

I would still suggest holding out until 16/17 for any throw-by-default behaviour though, but it could be beneficial to backport the warning actually.

I'm updating the PR description, but if anyone has any concerns let me know.

For all the modules experimental status process and changes, this is probably one of the only few removals we've done actually!

@guybedford
Copy link
Contributor Author

@addaleax given the above, would you be ok with removing the semver major label here?

@addaleax
Copy link
Member

@guybedford As I said, no strong opinion :) Just the fact that runtime deprecations are semver-major by default, but if everybody else is good with it then I am too

@guybedford
Copy link
Contributor Author

Just the fact that runtime deprecations are semver-major by default, but if everybody else is good with it then I am too

Ah I was not aware of that. Ok I'm completely fine with leaving the backport then.

@addaleax
Copy link
Member

@guybedford Is there a specific question?

@guybedford
Copy link
Contributor Author

@addaleax yes, would you be ok with removing the "semver-major" label from this issue?

@addaleax
Copy link
Member

@guybedford I still am, yes

@MylesBorins MylesBorins removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 30, 2020
@MylesBorins
Copy link
Contributor

Landed in c9acb9e

MylesBorins pushed a commit that referenced this pull request Oct 30, 2020
PR-URL: #35747
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
targos pushed a commit that referenced this pull request Nov 3, 2020
PR-URL: #35747
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@targos targos mentioned this pull request Nov 3, 2020
@dcleao
Copy link

dcleao commented Mar 16, 2021

Is there any discussion, somewhere, on the pros and cons of leading node.js away from the import maps specification?
Have we gotten to a point where change only occurs forced by others (yarn pushes npm; node pushes import maps; etc)?

@bmeck
Copy link
Member

bmeck commented Mar 16, 2021

@dcleao that doesn't seem directly related to this issue. However, you may be more interest in policies which have a more node tailored schema and you can generate from import maps (though import maps can't quite do everything policies do). @giltayar was stating he was going to try and write a converter ( https://github.com/giltayar/import-map-to-policy it seems ). Per if node is moving away from import maps, I'd say we are moving closer to them not further way. Exports are quite a different feature so that might be what is causing confusion.

@dcleao
Copy link

dcleao commented Mar 16, 2021

@bmeck How can it not be related to this issue? The connection is pretty clear to me.

The subpath folder change applies to both imports and exports. Exports are quite a similar feature to imports, mostly symmetrical in nature and design and which were afaik derived from the ideas of import maps.

You can read more about the other side of the coin here: WICG/import-maps#232 and WICG/import-maps#244.

@ljharb
Copy link
Member

ljharb commented Mar 16, 2021

@dcleao the thing that was deprecated here is not actually a feature import maps supports, because this one also exposes the entire directory, and every filecontained within (including extension/index lookup for CJS).

@dcleao
Copy link

dcleao commented Mar 16, 2021

I am sorry, I must be mistaken then. Is this issue not related with the deprecation of https://nodejs.org/api/packages.html#packages_subpath_folder_mappings ?
Weren't the referenced and now deprecated folder mappings as designed by import maps?
"foo/" would expose any module below "foo", at any level, whatever its extension.

@bmeck
Copy link
Member

bmeck commented Mar 16, 2021

@dcleao they were not designed by import maps, that might be the confusion.

@dcleao
Copy link

dcleao commented Mar 16, 2021

@bmeck They weren't designed to be identical to import maps, but I'm almost sure they were inspired, motivated by, triggered by, and, most importantly, designed to work with import maps.

Even if you insist that this was not the case, given the causal and proximal appearance, their syntactic and semantic similarity, and related talks by maintainers of both sides on both repositories, I'm sure this was how most people understood the relation of Node's imports and exports to Import Maps.

At a minimum, I'd expect being able to compile Node's imports and exports to Import Maps, for the subset of features which are expected to work and be useful on Node and the Browser. This included being able to convert a simple export folder mapping to an Import Map's, same syntax and semantics, import mapping. This being deprecated, in favor of pattern mappings, there's no longer a way to convert one to the other... It's broken, for the normal use case.

I'm still trying to digest how you can say these two technologies are unrelated...

@ljharb
Copy link
Member

ljharb commented Mar 16, 2021

You still can do that compilation.

this feature was designed as an escape hatch from the encapsulation and explicit requirements of the exports field. Using it makes it harder to generate an import map (although it was of course possible).

@bmeck
Copy link
Member

bmeck commented Mar 16, 2021

Even if you insist that this was not the case, given the causal and proximal appearance, their syntactic and semantic similarity, and related talks by maintainers of both sides on both repositories, I'm sure this was how most people understood the relation of Node's imports and exports to Import Maps.

I mean... policies predate import maps, but exports is after import maps; I do think the features are dealing with similar data and problem spaces but not trying to solve the same problems. The similarity though doesn't mean they are actually intertwined. Design choices like import maps having ambient authority to import vs policies having limited authority by default for example causes divergence. Choices around scoping on package boundaries for "exports" vs import maps being a flat namespace is another.

At a minimum, I'd expect being able to compile Node's imports and exports to Import Maps, for the subset of features which are expected to work and be useful on Node and the Browser. This included being able to convert a simple export folder mapping to an Import Map's, same syntax and semantics, import mapping. This being deprecated, in favor of pattern mappings, there's no longer a way to convert one to the other... It's broken, for the normal use case.

Unfortunately import maps are not tailored for various use cases so other features outside of them are being created to serve these purposes. This lack of features from import maps makes having a complete compatibility unlikely. I'd agree with @ljharb that compilation to import maps from other features is likely the approach to take.

@dcleao
Copy link

dcleao commented Mar 16, 2021

@ljharb

You still can do that compilation.

In fact, given the following folder pattern mapping:

"./foo/*": "./foo/*.js"

it can be converted to one or more "regular" folder mappings, if all of the js files below ./foo/, in sub-directories at any level, are mapped explicitly. The conversion exists, but using two quite different levels of abstraction, which I would hardly consider equivalent. However, yes, it can be said that there exists an equivalent mapping, in operational terms.

Yet, if there are 200 JS files below ./foo/, does the conversion lead to a practically usable Import Map?

this feature was designed as an escape hatch from the encapsulation and explicit requirements of the exports field. Using it makes it harder to generate an import map (although it was of course possible).

Unfortunately, I did not fully understand what you meant. What is "this feature" referring to? The "folder mappings" being deprecated? Or, the new "folder pattern mappings"?

@dcleao
Copy link

dcleao commented Mar 16, 2021

@bmeck

The similarity though doesn't mean they are actually intertwined.

In the sense that they become forced to evolve together — no, surely not. But would the ecosystem gain from "imports"/"exports" and "Import Maps" evolving together? Yes, I think it would.

Design choices like import maps having ambient authority to import vs policies having limited authority by default for example causes divergence

Policies deal with additional concerns compared to Import Maps.
Both "imports"/"exports" and Import Maps focus on the so called "dependency redirection" (in "Policies" terms).
This is not a justification for "imports"/"exports" to deviate from "Import Maps", but one for both to deviate from "Policies".

Choices around scoping on package boundaries for "exports" vs import maps being a flat namespace is another.

The concept of packages does not exist as a first class citizen in the "web", nor it does in Import Maps.
Yet, the names of packages simply form the first level of an equally "flat namespace". In Import Maps, the first level is occupied by import specifiers. Import Maps remains agnostic to the meaning of import specifiers (those which are not URLs); whether these represent NodeJs Packages or something else.

Package boundaries are somewhat erased when represented as Import Maps, but the same name mappings apply. Or, could you be referring to whether it is allowed to use parent relative paths (../) to refer to locations above a mapped specifier?

Unfortunately import maps are not tailored for various use cases so other features outside of them are being created to serve these purposes. This lack of features from import maps makes having a complete compatibility unlikely. I'd agree with @ljharb that compilation to import maps from other features is likely the approach to take.

In essence, both of the features, "imports" and "exports", have/had a corresponding equivalent representation in "Import Maps", apart from some minor differences.

There are some features of "imports" and "exports" that do not have a correspondence in Import Maps:

  1. condition maps (with "node", "default", "development", etc.) — could be handy, but is not essential for a runtime format
  2. fallback paths — was part of an initial version of Import Maps, but did not make it to the initial version.

It is only "Policies" which handles other quite distinct responsibilities.

So what other features or use cases are you referring to that justify these not evolving hand in hand?
(I'm excluding the change in this PR, as I'm exactly trying to establish that "imports"/"exports" had a close equivalent representation in "Import Maps", before this PR.)


I think that this discussion reveals that there could be value in a document clarifying the expected path of NodeJS's "imports" / "exports" features and the differences it has to "Import Maps".

@ljharb
Copy link
Member

ljharb commented Mar 16, 2021

Browsers have different constraints and affordances than node does, and as such, while it's useful for a number of reasons to maximize overlap, it wouldn't actually make sense to try to make either one exactly match the other. "exports" governs both CJS and ESM, for one, and browsers don't have CJS.

@bmeck
Copy link
Member

bmeck commented Mar 16, 2021

But would the ecosystem gain from "imports"/"exports" and "Import Maps" evolving together? Yes, I think it would.

I'm actually unclear on this. As shown beneath this statement the feature sets aren't the same so evolving them together would be around interoperability not around having the exact same data scheme. What does "evolving together" mean? Move all the work for node into WHATWG (browser) standards / not implement things unless they are in browsers? For more clarity, removing folder mappings like in this PR would bring "exports" closer to what import maps provides since it removes a feature that import maps don't support. "scopes" in import maps need to have a single destination for their specifier mappings.

This is not a justification for "imports"/"exports" to deviate from "Import Maps", but one for both to deviate from "Policies".

I don't understand this. Perhaps clarification on how they are the same as import maps would help. "imports" is pretty close, but "exports" is kind of the inverse of import maps to my perspective. I would agree that we don't have a need to deviate more than is necessary and don't really want to cause friction but I'm unclear on how we could be aiming to be more compatible except by removing features that may be intrinsic to things like how "exports" works.

Package boundaries are somewhat erased when represented as Import Maps, but the same name mappings apply. Or, could you be referring to whether it is allowed to use parent relative paths (../) to refer to locations above a mapped specifier?

I'm actually referring to specific cases where the behavior of "import"/"export" vary based upon non-compatible semantics from import maps. Things like conditions that you mention, fallbacks, etc. are pretty easy to understand but can't be exactly emulated by import maps. However, things like preventing resolution ("cascade" in policy terms) at package boundaries for "import" and allowing generalized exposing of subsets of directories are possible to do in import maps if you compile it out, but are quite verbose/hard to deal without some sort of tooling.

So what other features or use cases are you referring to that justify these not evolving hand in hand?
(I'm excluding the change in this PR, as I'm exactly trying to establish that "imports"/"exports" had a close equivalent representation in "Import Maps", before this PR.)

I'm unclear on "evolving hand in hand" here, WHATWG is a browser spec place that isn't exactly a place that Node can participate very well in. See various efforts by Guy Bedford ( https://github.com/guybedford/import-maps-extensions ) to try and expand the spec to be more feature complete with the tings we are talking about. However, matching the browser constraints and not implementing useful features for node is unlikely, and the same for browsers removing the demanded constraints for their environment.

It would be helpful to understand your suggestion that they evolve hand in hand. Like @ljharb I don't see being able to only accept features that the web allows as a valid path forward. Ensuring that you can generate import maps from Node's features seems somewhat feasible, but like stated above some features won't work so it would be on best effort.

I think that this discussion reveals that there could be value in a document clarifying the expected path of NodeJS's "imports" / "exports" features and the differences it has to "Import Maps".

I think such a thing would be great if someone wants to write it. I don't think it should be done so in an adversarial way though so biasing towards one or the other being "the right way" would be non-productive I think. They are just slightly different in purpose.

@dcleao
Copy link

dcleao commented Mar 19, 2021

Unfortunately, we're not being able to converge. Although somewhat repeating myself, I will try to summarize my point of view as clearly as I can.

  1. I'm focusing on the relation between NodeJS's imports and exports, and WICG import maps
    1.1. Specifically, on the concern of "module name mapping", both global and contextual (via scopes)
    1.2. I'm purposely avoiding other concerns, such as access control and package/scope isolation, integrity and dependency metadata, not because they're unrelated, or do not apply to both the Web and the NodeJS environment, but because they are not part of the current WICG import maps specification
    1.3. As noted by @bmeck, proposals exist which complement WICG import maps with these concerns, either as extensions to WICG import maps or as separate proposals
    1.4. NodeJS's Policies is one such proposal which handles most of these concerns, and which overlaps with NodeJS's own imports and exports, regarding "module name mapping"

  2. I'm specifically focusing the discussion on the common features between the Web and NodeJS environments
    2.1. Differences are expected wherever two environments are different (e.g. fallbacks?)
    2.2. Differences are undesirable and surprising wherever two environments can be seen as equal (e.g. different syntaxes or capabilities for declaring folder mappings)

  3. NodeJS's package concept does not exist on the Web (unfortunately, if you ask me)
    3.1. While the division between imports and exports makes all of the sense for packages, it does not if there is a single package, i.e., there are no packages
    3.2. However, conceptually, the imports and exports of a set of NodeJS packages can be transformed to a single WICG import map containing global and contextual imports
    3.3 A closed set of NodeJS's packages, expressed by {name + version + dependencies et al. + imports + exports} (all dependencies are included in the set) can be converted to one WICG import maps {imports}. The conversion is one way. Package information is lost. In a way, the Web environment and WICG import maps are currently lower level and more abstract, and can be seen as a compilation target

  4. The syntax used in NodeJS's imports and exports to declare "regular" folder mappings — the type removed by this PR, — is the syntax used to declare folder mappings in a WICG import map
    4.1. The new NodeJS's "folder pattern mappings" have no direct correspondence (at the same semantic level) in WICG import maps
    4.2. Although a conversion exists from the new NodeJS's "folder pattern mappings" to WICG import maps, by expanding the folder pattern mapping to one WICG folder mapping per matching NodeJS module, this generally results in bigger import maps (compared to the original representation), and makes developers lives harder w.r.t. to knowledge reuse
    4.3. This change increases the distance between NodeJS's imports and exports and WICG import maps

I'm looking forward for extensions to the current WICG import maps with concepts which apply to all environments, as well as with concepts which apply only to some environments. It seems reasonable to try to use the WICG import maps as the trunk on which at least some of these extensions will lie. For concepts which don't have an exact equivalent on some of the environments, I'd still expect that we'd strive for making useful conversions possible, and practically feasible.

I hope that this clarifies the expectations that I have for the future of NodeJS's packages and modules, and its relation to the Web's WICG import map and its future, related extensions. Maybe it's just me, maybe I'm just confused, but, also, maybe, I'm the only guy speaking out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.